Skip to content

Add pre-commit hooks and github workflow #1040

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Mar 28, 2023
Merged

Add pre-commit hooks and github workflow #1040

merged 10 commits into from
Mar 28, 2023

Conversation

@csc-felipe
Copy link
Contributor

@csc-felipe csc-felipe commented Mar 23, 2023

Description

Add pre-commit hooks and checks.

Also added a github action. If it works, and others agree, we could change a bit the github workflows and tox configuration.

Update your environment and enable the git hooks by running:

pip install -Ue .[docs,test,dev]
pre-commit install

Related issues

Type of change

  • New feature (non-breaking change which adds functionality)

Changes Made

  • Added pre-commit configuration
  • fixed raised issues from the checks
  • moved pyspelling configuration to root folder
  • use ruff instead of flake8 and isort

Testing

  • Tests do not apply

Mentions

It might be easier to review by looking at commits individually.

I tried to use the pyupgrade fixes from ruff, but that broke the tests, and I didn't want to investigate. There are a few other tests that might be interesting to consider in the future: e.g. tryceratops, which checks for how to handle exceptions.

@csc-felipe
Copy link
Contributor Author

csc-felipe commented Mar 23, 2023

Weird that mypy has different output when running with pre-commit

blankdots
blankdots reviewed Mar 27, 2023
View reviewed changes
Copy link
Contributor

@blankdots blankdots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I understand the need for both tox and pre-commit workflows

@blankdots
Copy link
Contributor

blankdots commented Mar 27, 2023

pre-commit run --all-files -c .pre-commit-config.yaml gives

ruff.....................................................................Failed
- hook id: ruff
- exit code: 1

swift_browser_ui/request/db.py:48:37: S311 Standard pseudo-random generators are not suitable for cryptographic purposes
swift_browser_ui/request/db.py:51:37: S311 Standard pseudo-random generators are not suitable for cryptographic purposes
swift_browser_ui/request/db.py:54:37: S311 Standard pseudo-random generators are not suitable for cryptographic purposes
swift_browser_ui/sharing/db.py:53:37: S311 Standard pseudo-random generators are not suitable for cryptographic purposes
swift_browser_ui/sharing/db.py:56:37: S311 Standard pseudo-random generators are not suitable for cryptographic purposes
swift_browser_ui/sharing/db.py:59:37: S311 Standard pseudo-random generators are not suitable for cryptographic purposes
swift_browser_ui/ui/middlewares.py:74:11: C901 `error_middleware` is too complex (14 > 10)
Found 7 errors.

Maybe we should fix those as well ?

@csc-felipe
Copy link
Contributor Author

csc-felipe commented Mar 27, 2023

not sure I understand the need for both tox and pre-commit workflows

pre-commit doesn't run pytest. And tox is good for running the same tests with different environment configuration. Also, keeping it there might show issues with the pre-commit configuration, as it's happening now.

@csc-felipe
Copy link
Contributor Author

csc-felipe commented Mar 27, 2023

pre-commit run --all-files -c .pre-commit-config.yaml gives

Did you run pre-commit autoupdate? Those checks are in a new version of ruff, and I kept the same version from the previous PR so I wouldn't have to fix those now.

@blankdots
Copy link
Contributor

blankdots commented Mar 27, 2023

Did you run pre-commit autoupdate? Those checks are in a new version of ruff, and I kept the same version from the previous PR so I wouldn't have to fix those now.

yes i did :), well i don't see why not update

@csc-felipe
Copy link
Contributor Author

csc-felipe commented Mar 27, 2023

Weird that mypy has different output when running with pre-commit

If I remove installing requirements.txt from tox, mypy doesn't complain. Adding it to pre-commit config doesn't work

@blankdots
Copy link
Contributor

blankdots commented Mar 27, 2023

If I remove installing requirements.txt from tox, mypy doesn't complain. Adding it to pre-commit config doesn't work

i think that is not the solution, as mypy is correct to complain at that line, i think the problem is in pre-commit, I saw some something related to: https://github.com/pre-commit/mirrors-mypy/issues/3

removing additional_dependencies and adding language system seems to show the errors, but the additional dependencies need to be installed separately

Base automatically changed from feature/vue3 to devel March 28, 2023 07:28
@csc-felipe csc-felipe force-pushed the feature/pre-commit-2 branch from 998e92e to 6c94057 Compare March 28, 2023 07:32
@csc-felipe csc-felipe force-pushed the feature/pre-commit-2 branch from 6c94057 to 3bbc83c Compare March 28, 2023 07:34
@csc-felipe
Copy link
Contributor Author

csc-felipe commented Mar 28, 2023

Rebased and fixed the mypy issue. Now it runs from locally installed mypy, and added type deps to pyproject.toml

@csc-felipe csc-felipe force-pushed the feature/pre-commit-2 branch 2 times, most recently from f270105 to 79f9fb4 Compare March 28, 2023 07:43
@blankdots
Copy link
Contributor

blankdots commented Mar 28, 2023

i am ok with the changes, but tests do need to work again.

I remember @csc-jm was working on them, maybe he is interested in this PR

@csc-felipe csc-felipe force-pushed the feature/pre-commit-2 branch from 79f9fb4 to f5d22a4 Compare March 28, 2023 09:08
@csc-felipe
Copy link
Contributor Author

csc-felipe commented Mar 28, 2023

Reverted the exception handling to previous code, and used a simpler approach to reduce complexity.

@csc-felipe csc-felipe merged commit 1b7ea83 into devel Mar 28, 2023
@csc-felipe csc-felipe deleted the feature/pre-commit-2 branch March 28, 2023 13:06
blankdots pushed a commit that referenced this pull request Jun 13, 2023
Merge branch 'bugfix/tagging-functionality' into 'devel'
a1311e3 
Fix for tagging functionality in subfolders and files

Closes #1040

See merge request sds-dev/sd-connect/swift-browser-ui!61
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hannyle hannyle Awaiting requested review from hannyle

@sampsapenna sampsapenna Awaiting requested review from sampsapenna

1 more reviewer

@blankdots blankdots blankdots approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@csc-felipe csc-felipe

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@csc-felipe @blankdots